Skip to content

fix: address Codex review findings on #382 and #390 (#95, #100) - #409

Merged
ScriptedAlchemy merged 3 commits into
mainfrom
fix/95-100-review-followups
Sep 3, 2026
Merged

fix: address Codex review findings on #382 and #390 (#95, #100)#409
ScriptedAlchemy merged 3 commits into
mainfrom
fix/95-100-review-followups

Conversation

@ScriptedAlchemy

Copy link
Copy Markdown
Owner

Summary

Follow-up to the automated review threads on already-merged PRs #382 and #390.

  • feat(routes): generate typed provider declarations and augment the runtime (#95) #382 thread (typegen.ts:39) — the generated augmentation makes declared provider keys required, but runAgentRequest and the route-unit/CLI/MCP harnesses never execute src/providers/*, so a custom scope or test could omit fixtures while (await agent()).providers.library was typed present. Now, once AgentProviderValues has required keys, AgentRequestInit.providers (AgentRequestProvidersInit), the harness options argument (HarnessOptionsArguments), and context (RenderRouteContextInit) become required in the same program. Provider-free projects and generated request scopes are unchanged.
  • feat(inspect): explain selected and omitted host components per target (#100) #390 thread (api.ts:594) — inspect spread adapter-owned CapabilityState rows into component accounting, so extension fields on a JavaScript/third-party adapter could shadow the canonical name or break inspect --json with a cycle. It now projects only the four-state contract fields and assigns the canonical name last.
  • README/docs updated; changeset required-provider-fixtures.

Evidence

  • packages/agent-bundle/tests/provider-typegen.test.ts: with declared providers, runAgentRequest({ invocation }) fails with Property 'providers' is missing; renderRoute(id, { context: { providers: { library } } }) fails with Property '"buildNumber"' is missing; renderRoute(id) fails with Expected 2 arguments, but got 1; a complete custom scope typechecks clean.
  • packages/agent-bundle/tests/api.test.ts "projects only the capability contract fields of adapter-owned rows into inspection": decorated rows with name: 'shadow' and a cyclic extension field yield exactly the contract fields with the canonical name and JSON.stringify succeeds.

Test plan

  • pnpm typecheck, pnpm lint
  • pnpm test:projection (63/63), pnpm test:route-unit (lifecycle-replay timeout reproduces on main; passes in isolation)
  • pnpm test:unit 2705 pass; 4 load-sensitive timeouts rerun green except native-claude-contract which fails identically on a worktree without these changes (environment-bound, green on main CI)
  • pnpm build && pnpm test:integration:run 946 pass; one workbench e2e timeout passes on rerun
  • CI green + Codex re-review threads addressed before merge

@changeset-bot

changeset-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: bd0f013

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@agent-bundle/runtime Minor
agent-bundle Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-03T08:39:29.056555Z bd0f013 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@pkg-pr-new

pkg-pr-new Bot commented Sep 3, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle@409
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/create-agent-bundle@409
npm i https://pkg.pr.new/ScriptedAlchemy/agent-bundle/@agent-bundle/runtime@409

commit: bd0f013

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Provided git ref 82113a551a206ddbe23c21e0ae0f2007d0cc7d86 does not exist
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 82113a551a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +2 to +3
"@agent-bundle/runtime": patch
"agent-bundle": patch

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use minor bumps for these breaking API changes

The acceptance test explicitly verifies that provider-enabled projects which previously compiled calls such as runAgentRequest({ invocation }) or renderRoute(id) now receive type errors, so this is a breaking public API change for both @agent-bundle/runtime and agent-bundle. Marking both releases as patch violates the repository's pre-1.0 policy and can deliver the break to consumers during a patch upgrade; change both entries to minor.

AGENTS.md reference: AGENTS.md:L25-L26

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — a provider-enabled project that compiled runAgentRequest({ invocation }) or renderRoute(id) now needs consumer action, which is the pre-1.0 minor case in AGENTS.md. Both entries are minor in 8637dec and the summary names the break.

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: 8637decf75

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ScriptedAlchemy
ScriptedAlchemy force-pushed the fix/95-100-review-followups branch from 8637dec to bd0f013 Compare September 3, 2026 08:32
@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: bd0f013d7b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ScriptedAlchemy
ScriptedAlchemy merged commit 62138ef into main Sep 3, 2026
11 checks passed
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
…ocument module-evaluation scope

Auto-mounting made #409's harness rule unreachable: once the generated
augmentation declared provider keys, HarnessOptionsArguments and
RenderRouteContextInit turned `options`/`context.providers` mandatory, so a
typed project could never let the harness mount its real providers. The harness
now keeps both optional (an explicit map must still carry every declared key;
a direct runAgentRequest still requires providers) and framework-mode.md,
entry-conventions.md, and the README describe the auto-mount contract instead
of "the harness never executes provider modules". They also record what the
harness does not simulate: provider modules are evaluated once per test worker,
so module-level provider state is shared across simulated executables and is
only proven cold by the proof levels that spawn the artifact.
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
…r provider typegen

The #409 acceptance pinned `renderRoute(id)` as a compile error once the
augmentation declares provider keys. With the harness mounting the project's
providers itself that call is the artifact-faithful one, so it now typechecks
clean alongside a call that passes only `input`, while a partial explicit
fixture still fails on the missing key and a direct runAgentRequest still
requires `providers`.
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
…ocument module-evaluation scope

Auto-mounting made #409's harness rule unreachable: once the generated
augmentation declared provider keys, HarnessOptionsArguments and
RenderRouteContextInit turned `options`/`context.providers` mandatory, so a
typed project could never let the harness mount its real providers. The harness
now keeps both optional (an explicit map must still carry every declared key;
a direct runAgentRequest still requires providers) and framework-mode.md,
entry-conventions.md, and the README describe the auto-mount contract instead
of "the harness never executes provider modules". They also record what the
harness does not simulate: provider modules are evaluated once per test worker,
so module-level provider state is shared across simulated executables and is
only proven cold by the proof levels that spawn the artifact.
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
…r provider typegen

The #409 acceptance pinned `renderRoute(id)` as a compile error once the
augmentation declares provider keys. With the harness mounting the project's
providers itself that call is the artifact-faithful one, so it now typechecks
clean alongside a call that passes only `input`, while a partial explicit
fixture still fails on the missing key and a direct runAgentRequest still
requires `providers`.
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
ScriptedAlchemy added a commit that referenced this pull request Sep 3, 2026
…der mounting and deterministic examples-real edits (#399)

* feat(test): mount conventional providers in the agent-bundle/test harness

The public harness (renderRoute, renderRouteEvents, invokeCli, in-memory MCP)
now discovers and executes src/providers/* for every manifest-backed request
scope exactly like the generated entries: same deterministic key order, same
surface-specific invocation, same fail-closed messages, seeded processLifetime.
context.providers still wins when passed. The execution contract the codegen
emits and the harness runs now lives in one module and is pinned together.

Refs #313, #366.

* test(workbench): wait on the watcher rebuild instead of retrying examples-real edits

Each source edit paired a file write with an immediate manual rebuild, so the
watcher's own rebuild of the same write raced it for a second epoch whose
timing depended on load; { retry: 2 } absorbed the fallout. Edits now go
through replaceWatchedSourceAndAwaitRebuild: one atomic replacement, then a
wait on the coordinator's published build attempt, so one edit is exactly one
build and the retries are gone.

Refs #122, #200, #329.

* test(route-unit): reconcile the #371 provider seam pin with harness auto-mounting

Explicit context.providers still mounts verbatim; a module rendered directly
now observes the framework-owned processLifetime like a generated scope
without providers, and manifest routes execute conventional providers.

* test(workbench): replace the logs-real source edit atomically

A truncating writeFile can split into two watcher invalidations under load,
logging "Project source changed." twice and tripping the strict locator; the
shared atomic replacement makes one edit one invalidation.

* fix(test): derive executable surface for harness invocations and bump registry version

renderRoute now hands providers and the request scope the surface the
generated entries record — a routed CLI command's space-joined command path
and a script's path-derived name — instead of the route id, so providers that
branch on `command`/`name` behave identically in the harness and the artifact.
The test registry version moves to 4 because the layout gained
`providerLoaders`. The audiobook-curator degraded-catalog test opts out of the
auto-mounted library provider explicitly.

* fix(test): pin the tooling tool in the packed projection and ignore example artifacts

The packed stdio projection lists the route-harness tools, which now include
`tooling`. Every example's build writes `examples/<name>/artifact/`; the
previous commit accidentally tracked three of them, so ignore the whole
family and untrack the accidental copies.

* fix(build): mount the compiled event route id as operationId in the Flight worker

The generated worker resolved event routes by their hook identity and mounted
that identity (`hook:event-route:tool-after`) as `invocation.operationId`,
while the hook shell's request scope, the lifecycle replay, the test manifest,
and `renderRoute` all use the compiled route id (`event:tool/after`). The
worker record now carries the compiled id, so a route reading
`invocation.operationId` sees one value on every surface; pinned by the
warm-runtime integration test and the worker source digest.

* fix(test): scope the harness process lifetime to each simulated executable

A module-level lifetime made unrelated invokeCli calls, renders, and MCP
sessions look like one warm process, so a provider branching on hits or
instanceId could pass in the harness and fail in the artifact. The lifetime now
lives on the logical executable exactly as the artifact scopes it: fresh per
CLI invocation and per route-unit render, shared across the requests of one
open in-memory MCP server, and fresh again for the open-call-close helper.

* fix(test): snapshot the process hit count before awaiting provider loaders

Concurrent requests on one in-memory MCP server could each increment the shared
lifetime before the first request reached executeProviders, so every request
observed the final count. Capture each request's hit right after the
increment, as the generated worker does, and hand the snapshot to the shared
execution helper.

* fix(build,test): snapshot the process hit synchronously before state bindings

Generated stateful scopes incremented processLifetime.hits, awaited state
bindings, then snapshotted the value, so concurrent requests could observe the
same count; the harness's in-memory server claimed its hit only after the
bindings resolved. Both now claim and snapshot in one synchronous step before
any await: the emitted scopes bind `processHit` at the increment, and the
harness claims through claimProcessHit before requestBindings. The harness also
looks up CLI command paths among authored commands only, since projected MCP
commands carry their tool's route id and render through the tool branch.

* fix(test): keep harness context optional under provider typegen and document module-evaluation scope

Auto-mounting made #409's harness rule unreachable: once the generated
augmentation declared provider keys, HarnessOptionsArguments and
RenderRouteContextInit turned `options`/`context.providers` mandatory, so a
typed project could never let the harness mount its real providers. The harness
now keeps both optional (an explicit map must still carry every declared key;
a direct runAgentRequest still requires providers) and framework-mode.md,
entry-conventions.md, and the README describe the auto-mount contract instead
of "the harness never executes provider modules". They also record what the
harness does not simulate: provider modules are evaluated once per test worker,
so module-level provider state is shared across simulated executables and is
only proven cold by the proof levels that spawn the artifact.

* test(typegen): pin that harness calls stay legal without context under provider typegen

The #409 acceptance pinned `renderRoute(id)` as a compile error once the
augmentation declares provider keys. With the harness mounting the project's
providers itself that call is the artifact-faithful one, so it now typechecks
clean alongside a call that passes only `input`, while a partial explicit
fixture still fails on the missing key and a direct runAgentRequest still
requires `providers`.

* fix(test,changeset): scale the watcher e2e outer timeouts and rewrite the changeset as a release summary

The three examples-real tests that wait on watcher rebuilds bounded their
rebuild waits at 60s × timeScale but kept fixed 120s/150s outer timeouts, so
in CI (timeScale 4) Rstest could end the test before its own readiness wait
did. Their outer timeouts scale the same way now. The changeset is rewritten
per AGENTS.md as an imperative user-facing summary naming the harness exports
and the harness error, ending with the PR reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant